-
Notifications
You must be signed in to change notification settings - Fork 13.3k
Use PlaceRef projection abstractions more consistently in rustc_mir #80865
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Conversation
@@ -478,7 +478,7 @@ impl<'tcx> Validator<'_, 'tcx> { | |||
// https://github.com/rust-lang/rust/pull/74945#discussion_r463063247 | |||
// There may be opportunity for generalization, but this needs to be | |||
// accounted for. | |||
if proj_base.is_empty() | |||
if place_base.projection.is_empty() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the is_empty
check should be moved up near self.temps[place_base.local]
-- that code only makes sense if there are no projections.
Thanks a lot for this PR, @oliviacrain. :) I just left two minor comments; everything else looks perfect! |
Thanks for the comments, @RalfJung! I've addressed comments, squashed fixups, and rebased against master. |
Thanks for the quick update! However, for the future, it'd be better if you could refrain from force-pushing to a branch until review is done. With a force-push, now I have to re-read the entire PR again, I have no way of checking just the new diff. If there are conflicts, a rebase (+ force-push) is needed, but squashing should never be done until review is complete. If you hadn't rebased I could have used GH's force-push-diff link, but after squash + rebase, all hope of an incremental review is lost.^^ |
Ah, so sorry about that. My brain didn't switch from work PR mode... will avoid that in the future. |
No worries, different policies in different projects can be confusing. ;) |
@@ -1007,27 +1007,26 @@ fn place_as_reborrow( | |||
body: &Body<'tcx>, | |||
place: Place<'tcx>, | |||
) -> Option<&'a [PlaceElem<'tcx>]> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks to me like the return type should be changed to Option<PlaceRef>
... but that's a larger change, no need to do it in this PR.
This looks great, thanks for working on this. :) |
📌 Commit 65b5e43 has been approved by |
Use PlaceRef projection abstractions more consistently in rustc_mir PlaceRef contains abstractions for dealing with the `projections` array. This PR uses these abstractions more consistently within the `rustc_mir` crate. See associated issue: rust-lang#80647. r? `@RalfJung`
☀️ Test successful - checks-actions |
PlaceRef contains abstractions for dealing with the
projections
array. This PR uses these abstractions more consistently within therustc_mir
crate.See associated issue: #80647.
r? @RalfJung